Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(templates/arc): update README + dependencies #2213

Closed
wants to merge 4 commits into from

Conversation

ryanblock
Copy link

Closes: #

  • Docs
  • Tests

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 4, 2022

Hi @ryanblock,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 4, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@@ -8,6 +8,7 @@
"start": "cross-env NODE_ENV=production arc sandbox"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcansh: NODE_ENV=development is not required for any Architect processes, so in theory we can remove that from start + dev:arc if that doesn't cause any trouble for y'all!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not 100% sure when we added that to the templates, but i'm 95% sure we set NODE_ENV during remix dev and remix build (and remix-serve)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so do you want to leave it? Remove it? Arc is good either way, but imo always good to have less stuff if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove it

@@ -8,6 +8,7 @@
"start": "cross-env NODE_ENV=production arc sandbox"
},
"dependencies": {
"@architect/architect": "^10.0.5",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we help ensure this is relatively up to date?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't we use ^10.x to signify any version of v10? we don't include lock files in the templates so once they're installed they should use the latest version available, no?

Copy link
Collaborator

@mcansh mcansh Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise, we can keep it ^10.0.5 and wait for PRs to bump the version when anything significant happens

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can def safely set to 10.x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^10.0.0 should indeed just work


You will be running two processes during development when using Architect as your server.
### TypeScript
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does all this make sense in the context of Remix? I know a lot of Remix + Arc folks have been trying to use TS, but have been a bit confused as to where to start, so I was thinking this might help shortcut them. Up to y'all!

Copy link
Collaborator

@mcansh mcansh Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately using @architect/plugin-typescript for all lambdas causes a "lambda not found error" for the remix app, perhaps we can mention the config.arc approach instead? we might be able to tweak some remix config to make it work, I'll give it a shot soon

Copy link
Collaborator

@mcansh mcansh Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol wut, today it works - reason being i tried upgrading an existing app yesterday that wasnt using the new serverBuildTarget to bundle the app into the server directory so arc was then trying to compile it too

Copy link
Collaborator

@mcansh mcansh Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think instead of mentioning the installation of the typescript plugin directly, we can add a arc-ts directory and add the dependency and full app.arc there

@mcansh mcansh added the needs-response We need a response from the original author about this issue/PR label Mar 16, 2022
@mcansh
Copy link
Collaborator

mcansh commented Mar 30, 2022

@ryanblock can you make these adjustments?

mcansh added a commit that referenced this pull request Mar 30, 2022
readme changes taken from #2213 as I can't push to that branch

Signed-off-by: Logan McAnsh <logan@mcan.sh>
@MichaelDeBoey MichaelDeBoey changed the title Readme + dependency updates fix(templates): update README + dependencies Mar 30, 2022
@MichaelDeBoey MichaelDeBoey changed the title fix(templates): update README + dependencies fix(templates/arc): update README + dependencies Mar 30, 2022
@MichaelDeBoey
Copy link
Member

@ryanblock Also make sure to rebase your branch onto latest main, as we've moved some things around in the meantime.

@machour
Copy link
Collaborator

machour commented May 23, 2022

@ryanblock #2332 got merged and implemented some README changes.
Can you have a look and update your PR accordingly?

Thank you 🙏🏼

@MichaelDeBoey
Copy link
Member

@ryanblock Could you please rebase your branch onto latest main & resolve conflicts?

@MichaelDeBoey MichaelDeBoey added needs-response We need a response from the original author about this issue/PR and removed needs-response We need a response from the original author about this issue/PR labels Jun 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

This PR has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from PRs that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this Nov 3, 2022
@MichaelDeBoey MichaelDeBoey removed the needs-response We need a response from the original author about this issue/PR label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants